Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Initial setup for Amsterdam theme #2633

Merged
merged 12 commits into from
Dec 16, 2019
Merged

Initial setup for Amsterdam theme #2633

merged 12 commits into from
Dec 16, 2019

Conversation

johnbarrierwilson
Copy link
Member

@johnbarrierwilson johnbarrierwilson commented Dec 11, 2019 •
edited
Loading

Summary

Additions needed to support the new "Amsterdam" theme. Changes are made in both the source and in the documentation.

Additions include:

  • Registers and adds Amsterdam: Light and Amsterdam: Dark as options in the theme selector
  • Creates new stylesheets for Amsterdam light and dark
  • Make some minor changes to styling via the new stylesheets (so that theme selector can be tested)
    • Removes panel borders
    • Makes blue more saturated
  • Adds information to CONTRIBUTING.md regarding how to help contribute to Amsterdam
  • Adds checkmark to pull request template for checking changes against any new themes

Question

Should we even expose the new theme in the documentation? Or do we want to hide it behind a flag of some sort?

Checklist

  • Checked in dark mode
    - [ ] Checked in mobile
  • Checked in IE11 and Firefox
    - [ ] Props have proper autodocs
  • Added documentation examples
    - [ ] Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
    - [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@johnbarrierwilson johnbarrierwilson added the documentation Issues or PRs that only affect documentation - will not need changelog entries label Dec 11, 2019
@johnbarrierwilson johnbarrierwilson changed the title [WIP] Initial setup for Amsterdam theme Initial setup for Amsterdam theme Dec 11, 2019
@snide
Copy link
Contributor

snide commented Dec 11, 2019

Should we even expose the new theme in the documentation? Or do we want to hide it behind a flag of some sort?

Yes. It's fine to expose.

Adds information to CONTRIBUTING.md regarding how to help contribute to Amsterdam

I did not see this. You might have missed a commit.

@snide
Copy link
Contributor

snide commented Dec 11, 2019

I would suggest adding a new check mark onto https://github.com/elastic/eui/blob/master/.github/pull_request_template.md to reference that changes were checked against the new theme.

@johnbarrierwilson
Copy link
Member Author

Ah! Yea, I'll add a check mark.

Weird! I had a strange push issue. I pushed, but it didn't like my upstream setup. Anyway, that should be included now. Just some minor tweaks to themeing.md

@cchaos
Copy link
Contributor

cchaos commented Dec 12, 2019

The colors guideline page is not correctly pulling in the colors for the Amsterdam theme. You will want to look at this file to ensure the right colors file is being imported.

Screen Shot 2019-12-12 at 15 55 13 PM

@snide
Copy link
Contributor

snide commented Dec 12, 2019

The colors guideline page is not correctly pulling in the colors for the Amsterdam theme.

That file should give you what you need. You'll want to import the new theme at the top using the sass loader, and then trigger it here.

https://github.com/elastic/eui/blob/master/src-docs/src/views/guidelines/colors.js#L155-L156

A similar check should be done on this page here.

https://github.com/elastic/eui/blob/master/src-docs/src/views/guidelines/sass.js

cchaos
cchaos suggested changes Dec 12, 2019
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ The scss imports are working
✅ The theme switcher is working
✅ The css and json files get compiled during build
✖️ The colors and sass guideline pages aren't pulling in the amsterdam colors
Q: What is the wiki/theme-development.md file supposed to contain? It is currently an empty page.

.github/pull_request_template.md Outdated Show resolved Hide resolved
src/themes/eui-amsterdam/global_styling/mixins/_panel.scss Outdated Show resolved Hide resolved
wiki/theming.md Outdated Show resolved Hide resolved
wiki/theming.md Outdated Show resolved Hide resolved
src/themes/eui-amsterdam/global_styling/index.scss Show resolved Hide resolved
src/themes/eui-amsterdam/global_styling/mixins/_index.scss Show resolved Hide resolved
@johnbarrierwilson
Copy link
Member Author

Ah! 🤦🏻‍♂️ That file (theme-development.md) wasn't supposed to be committed.

@johnbarrierwilson
Copy link
Member Author

@cchaos These items have been taken care of now. Thanks again for the help! Let me know if there's anything else.

cchaos
cchaos approved these changes Dec 16, 2019
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making those changes! Still had one comment about the PR checklist but other than that, LGTM 👍

.github/pull_request_template.md Outdated Show resolved Hide resolved
@johnbarrierwilson johnbarrierwilson merged commit c7e7e94 into elastic:master Dec 16, 2019
@johnbarrierwilson johnbarrierwilson deleted the amsterdam/initial-setup branch December 16, 2019 22:56
@thompsongl thompsongl mentioned this pull request Dec 19, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snide snide snide left review comments

@cchaos cchaos cchaos approved these changes

Assignees
No one assigned
Labels
documentation Issues or PRs that only affect documentation - will not need changelog entries
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@johnbarrierwilson @snide @cchaos